-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: Deprecate dayofweek/hello day_of_week (#9606) #37390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLN: Deprecate dayofweek/hello day_of_week (#9606) #37390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abhishekmangla for the PR!
Looks good, some comments.
You'll also need to add a whatsnew (v1.2) under enhancements
Let us know if you have any questions!
@arw2019 Thank you for the review. I addressed your comments and added an entry to whatsnew file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on this!
Some more (minor) comments, otherwise good to go IMO
@arw2019 Resolved minor issues. Do you know how to locally test .rst files? In the CI stage of the checks, I am failing things like
|
I think you (maybe) need to add the new methods to the rst files manually - for example in doc/source/reference/series.rst both daysinmonth and days_in_month are listed
This one is a linting issue |
Rename dayofweek properties in Period, Datetime, and PeriodArray classes. Add day_of_week property to respective tests for each class.
Alias dep. function instead of extra property Remove comments from test files
Similar to changes for day_of_week, create aliases for day_of_year and add to respective test files to ensure back-compat. Also fix whatsnew rst file
7727dde
to
84be52f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine. ping on green.
doc/source/whatsnew/v1.2.0.rst
Outdated
@@ -13,14 +13,23 @@ including other versions of pandas. | |||
Enhancements | |||
~~~~~~~~~~~~ | |||
|
|||
|
|||
|
|||
.. _whatsnew_120.improve_timeseries_accessor_naming: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just put these notes in Other Enhancements section
doc/source/whatsnew/v1.2.0.rst
Outdated
:class:`Series` and :class:`DataFrame` can now be created with ``allows_duplicate_labels=False`` flag to | ||
control whether the index or columns can contain duplicate labels (:issue:`28394`). This can be used to | ||
prevent accidental introduction of duplicate labels, which can affect downstream operations. | ||
:class:`Series` and :class:`DataFrame` can now be created with ``allows_duplicate_labels=False`` flag to control whether the index or columns can contain duplicate labels (:issue:`28394`). This can be used to prevent accidental introduction of duplicate labels, which can affect downstream operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls don't adjust other notes
@pandas-dev/pandas-core should we do a DeprecationWarning / FutureWarning on the original attributes? (would be a followon) |
@jreback All checks green except aforementioned |
thanks @abhishekmangla very nice |
Rename dayofweek properties in Period, Datetime, and PeriodArray classes.
Add day_of_week property to respective tests for each class.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff